-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Message Infrastructure #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Looks good overall. Feedback is mostly about formatting and consistency.
However, there is one bug around set_var/get_var that should be looked at... It seems that this won't work right now when 'idx' is provided.
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py
Outdated
Show resolved
Hide resolved
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py
Outdated
Show resolved
Hide resolved
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Marcus G K Williams <[email protected]>
Source now moved from /lava to /src/lava. I'll need to push an update to fix this before we can merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awintel @jlakness-intel I've merged latest into this and resolved most of Andreas left over issues that were not too involved.
We should probably have @joyeshmishra or one of us address further issues in a future change:
- Refactor Message Infrastructure #29 (comment)
- Refactor Message Infrastructure #29 (comment)
- Refactor Message Infrastructure #29 (comment)
- https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r743942155
- https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r743994391
- https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r744005895
- https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r744005534
Per the conversation at WG do we want to merge this? We can then base all inflight code on it.
Signed-off-by: Marcus G K Williams <[email protected]>
I think it's fine to merge. Apparently none of the existing unit tests broke. The bug that I pointed out in the way set/get is implemented can also be addresses as part of a new bug issue after merging this one to main. |
@awintel can you approve to enable merge? |
* Refactor Message Infrastructure
No description provided.